Skip to content

refactor(network): Clean up utility functions for network commands#2725

Open
Caball009 wants to merge 8 commits into
TheSuperHackers:mainfrom
Caball009:refactor_network_if_else_chains
Open

refactor(network): Clean up utility functions for network commands#2725
Caball009 wants to merge 8 commits into
TheSuperHackers:mainfrom
Caball009:refactor_network_if_else_chains

Conversation

@Caball009
Copy link
Copy Markdown

@Caball009 Caball009 commented May 17, 2026 •
edited
Loading

This PR cleans up some utility functions in the network code, mainly by refactoring them from if/else chains to switch statements. No change should have a functional difference.

See commits for cleaner (per function) diffs.

@Caball009 Caball009 added Minor Severity: Minor < Major < Critical < Blocker Network Anything related to network, servers Refactor Edits the code with insignificant behavior changes, is never user facing labels May 17, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 17, 2026 •
edited
Loading

Greptile Summary

This PR refactors four network utility functions in NetworkUtil.cpp from verbose if/else chains to switch statements, adds const correctness to two function signatures, and introduces a CASE_LABEL helper macro in GetNetCommandTypeAsString to reduce boilerplate.

  • DoesCommandRequireACommandID and IsCommandSynchronized are converted to switch statements with no change to the set of matching enum values.
  • CommandRequiresAck is simplified to delegate directly to DoesCommandRequireACommandID, which is correct since both functions historically matched on the identical set of command types (the duplicate NETCOMMANDTYPE_DISCONNECTPLAYER entry in the old if-chain was a copy-paste artifact with no behavioral effect).
  • GetNetCommandTypeAsString now handles previously missing values (NETCOMMANDTYPE_UNKNOWN, NETCOMMANDTYPE_DISCONNECTSTART, NETCOMMANDTYPE_DISCONNECTEND) and restores the DEBUG_CRASH guard for truly unrecognized types (addressing the feedback from the prior review thread).

Confidence Score: 5/5

Safe to merge — all refactored switch statements cover the same enum values as the original if/else chains, and the addressed feedback from prior threads is correctly incorporated.

Each refactored function was verified to cover the same command types as the original. The CommandRequiresAck delegation to DoesCommandRequireACommandID is correct since the two functions had identical (modulo a copy-paste duplicate) type lists. The GetNetCommandTypeAsString function now properly handles NETCOMMANDTYPE_UNKNOWN and restores the DEBUG_CRASH guard for future unrecognized types. The const corrections in the header are straightforward and non-breaking.

No files require special attention.

Important Files Changed
Filename Overview
Core/GameEngine/Include/GameNetwork/networkutil.h Added const correctness to CommandRequiresAck and CommandRequiresDirectSend parameter declarations — minimal, correct change.
Core/GameEngine/Source/GameNetwork/NetworkUtil.cpp Refactored four functions from if/else chains to switch statements; CommandRequiresAck now delegates to DoesCommandRequireACommandID (lists were semantically identical); GetNetCommandTypeAsString adds CASE_LABEL macro, covers previously missing enum values (DISCONNECTSTART, DISCONNECTEND, UNKNOWN), and restores DEBUG_CRASH on unhandled types.

Flowchart

 
%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[NetCommandMsg] -->|getNetCommandType| B{NetCommandType}

    B --> C[CommandRequiresAck]
    B --> D[CommandRequiresDirectSend]
    B --> E[DoesCommandRequireACommandID]
    B --> F[IsCommandSynchronized]
    B --> G[GetNetCommandTypeAsString]

    C -->|delegates to| E

    E -->|FRAMEINFO, GAMECOMMAND, PLAYERLEAVE
RUNAHEADMETRICS, RUNAHEAD, DESTROYPLAYER
CHAT, LOADCOMPLETE, TIMEOUTSTART, WRAPPER
FILE, FILEANNOUNCE, FILEPROGRESS
FRAMERESENDREQUEST, DISCONNECTPLAYER
DISCONNECTVOTE, DISCONNECTFRAME
DISCONNECTSCREENOFF| E1[TRUE]
    E -->|default| E2[FALSE]

    D -->|LOADCOMPLETE, TIMEOUTSTART, FILE
FILEANNOUNCE, FILEPROGRESS
FRAMERESENDREQUEST, DISCONNECTPLAYER
DISCONNECTVOTE, DISCONNECTFRAME
DISCONNECTSCREENOFF| D1[TRUE]
    D -->|default| D2[FALSE]

    F -->|FRAMEINFO, GAMECOMMAND
PLAYERLEAVE, RUNAHEAD
DESTROYPLAYER| F1[TRUE]
    F -->|default| F2[FALSE]

    G -->|known type| G1[string literal via CASE_LABEL macro]
    G -->|unknown type| G2[DEBUG_CRASH + return NETCOMMANDTYPE_INVALID]
Loading

Reviews (8): Last reviewed commit: "Moved default case for 'GetNetCommandTyp..." | Re-trigger Greptile

greptile-apps[bot]
greptile-apps Bot reviewed May 17, 2026
View reviewed changes
Comment thread Core/GameEngine/Source/GameNetwork/NetworkUtil.cpp Outdated
Mauller
Mauller reviewed May 18, 2026
View reviewed changes
Comment thread Core/GameEngine/Source/GameNetwork/NetworkUtil.cpp
Mauller
Mauller reviewed May 18, 2026
View reviewed changes
Comment thread Core/GameEngine/Source/GameNetwork/NetworkUtil.cpp
Mauller
Mauller reviewed May 18, 2026
View reviewed changes
Comment thread Core/GameEngine/Source/GameNetwork/NetworkUtil.cpp
greptile-apps[bot]
greptile-apps Bot reviewed May 18, 2026
View reviewed changes
Comment thread Core/GameEngine/Source/GameNetwork/NetworkUtil.cpp
@Caball009 Caball009 force-pushed the refactor_network_if_else_chains branch from 4d28261 to 4d43c80 Compare May 18, 2026 17:05
xezon
xezon reviewed May 18, 2026
View reviewed changes
Comment thread Core/GameEngine/Source/GameNetwork/NetworkUtil.cpp Outdated
xezon
xezon reviewed May 18, 2026
View reviewed changes
Comment thread Core/GameEngine/Source/GameNetwork/NetworkUtil.cpp Outdated
@Caball009 Caball009 force-pushed the refactor_network_if_else_chains branch from daef210 to 9746679 Compare May 18, 2026 20:49
@Caball009 Caball009 force-pushed the refactor_network_if_else_chains branch from 9746679 to 58df0fe Compare May 19, 2026 22:14
@Caball009
Copy link
Copy Markdown
Author

Caball009 commented May 19, 2026

Rebased to include the fix for the CI Replay checker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mauller Mauller Mauller left review comments

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

@xezon xezon xezon approved these changes

Assignees

No one assigned

Labels

Minor Severity: Minor < Major < Critical < Blocker Network Anything related to network, servers Refactor Edits the code with insignificant behavior changes, is never user facing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Caball009 @xezon @Mauller